Move serialization back out of SplitShardCountSummary #136055
Conversation
Serialization of this field is determined by the serialization of the request in which it is embedded, so it cannot sensibly serialize itself.
bcully
added
>non-issue
:Distributed Indexing/Distributed
elasticsearchmachine
added
the
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the serialization logic for SplitShardCountSummary by moving the serialization responsibility from the summary class itself to the containing ReplicationRequest class. The change simplifies the design by making serialization dependent on the request's transport version rather than having the summary handle its own serialization.
Key changes:
- Remove
Writeableinterface implementation fromSplitShardCountSummary - Add static factory method and integer conversion methods to
SplitShardCountSummary - Move transport version constants and serialization logic to
ReplicationRequest
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SplitShardCountSummary.java | Removes serialization capabilities and adds factory/conversion methods |
| ReplicationRequest.java | Takes over serialization responsibility with transport version handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // visible for testing | ||
| SplitShardCountSummary(int shardCountSummary) { |
There was a problem hiding this comment.
[nitpick] The constructor visibility was changed from package-private with a comment 'visible for testing' to package-private without comment. Consider adding back the visibility comment or making it private since it's now only used internally via the fromInt() factory method.
| SplitShardCountSummary(int shardCountSummary) { | |
| private SplitShardCountSummary(int shardCountSummary) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I didn't mean to remove the comment. Thanks copilot!
| return shardCountSummary; | ||
| } | ||
|
|
||
| private final int shardCountSummary; |
There was a problem hiding this comment.
[nitpick] The field declaration was moved after the method definitions. Consider keeping field declarations at the top of the class for better readability and consistency with Java conventions.
Copilot uses AI. Check for mistakes.
Serialization of this field is determined by the serialization of the request in which it is embedded, so it cannot sensibly serialize itself.